Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support nativeTarget to build Scala Native static/shared libraries #2196

Merged
merged 11 commits into from
Oct 11, 2023
Merged

Support nativeTarget to build Scala Native static/shared libraries #2196

merged 11 commits into from
Oct 11, 2023

Conversation

keynmol
Copy link
Contributor

@keynmol keynmol commented Jun 9, 2023

Scala Native CLI supports building dynamic and shared libraries using --build-target argument.

So far it hasn't been supported. This PR adds the support, by

  • Adding nativeTarget directive and native-target CLI option
  • Splitting PackageType.Native into 3 different types
  • Modifying the logic to not throw when main class is not found if we are building a library
  • IMPORTANT: use -Wl,install_name argument to rename the library inside its own metadata, otherwise it points to .scala-build/project_asdkljadljk190/main - instead of test.dylib that we want

lwronski
lwronski reviewed Jun 12, 2023
View reviewed changes
Copy link
Contributor

@lwronski lwronski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks promising to me, and I would be happy to merge it. Unfortunately, there is one new test that is failing. You can find more details here.

To fix the checks and reference-doc jobs, run the following commands before pushing your changes:

./mill -i __.fix
./mill -i generate-reference-doc.run

modules/cli/src/main/scala/scala/cli/commands/package0/Package.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/package0/Package.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/package0/Package.scala Outdated Show resolved Hide resolved
@keynmol
Copy link
Contributor Author

keynmol commented Jun 12, 2023

@lwronski, thanks, that's exactly the commands I was looking for :)

If you are happy with the overall approach I will start fixing and cleaning up the PR

@lwronski
Copy link
Contributor

@lwronski, thanks, that's exactly the commands I was looking for :)

If you are happy with the overall approach I will start fixing and cleaning up the PR

Yes, I'm satisfied with the overall approach, so I will wait for the final results.

@Gedochao
Copy link
Contributor

Gedochao commented Oct 5, 2023

@keynmol what is the current status on this? can we unblock you somehow?

@keynmol
Copy link
Contributor Author

keynmol commented Oct 5, 2023

Hey @Gedochao! I didn't forget about this PR, just was short on time.

I resolved the conflicts (but incorrectly it seems, I will address those failures).

Now that I re-ran the tests on Linux, I remember the main problem - this PR relies on passing -install_name linker flag, to make sure that the dynamic library loading entries refer to the correct (relative) location of the built dynamic library, instead of the location where it's actually built (.scala-build/.../).

Problem is - on Linux this flag doesn't seem to work

Just as I was writing those last two paragraphs I found the linux equivalent option 🤦

I'll pick this up again and hopefully will address remaining test failures.

@Gedochao Gedochao requested a review from MaciejG604 October 9, 2023 14:00
@keynmol keynmol mentioned this pull request Oct 9, 2023
Merged
@Gedochao Gedochao force-pushed the scala-native-build-target branch from f30c1c5 to c17c9a1 Compare October 10, 2023 07:15
MaciejG604
MaciejG604 approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@MaciejG604 MaciejG604 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Gedochao Gedochao merged commit 8263c39 into VirtusLab:main Oct 11, 2023
38 checks passed
@keynmol
Copy link
Contributor Author

keynmol commented Oct 11, 2023

🎉

Thanks @Gedochao and @MaciejG604

@keynmol keynmol deleted the scala-native-build-target branch October 11, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lwronski lwronski lwronski left review comments

@WojciechMazur WojciechMazur WojciechMazur left review comments

@lolgab lolgab lolgab left review comments

@MaciejG604 MaciejG604 MaciejG604 approved these changes

@Gedochao Gedochao Gedochao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@keynmol @lwronski @Gedochao @lolgab @WojciechMazur @MaciejG604